drivers: display: ssd16xx: implement display_set_orientation #73360

Snevzor · 2024-05-27T15:01:56Z

I chose this approach instead of software rotation because I need it for an ultra low power project.
Rotation in steps of 0, 90, 180, 270 degrees is done by changing
the data entry modes.
This PR removes the orientation-flipped property, as it is redundant.

There's also a performance improvement in a separate commit.

Tested on the reel_board with a 250x122 display (ssd1673 controller)
and a custom PCB with a 200x200 display (ssd1681 controller).
Also tested all orientations with various width/height dts overrides.

To test with the LVGL sample, I had to change some glue code as well, see #73474

github-actions · 2024-05-27T15:02:38Z

Hello @Snevzor, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

decsny · 2024-05-28T16:40:58Z

dts/bindings/display/solomon,ssd16xx-common.yaml

default needs justification

https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#rules-for-default-values

Thanks for checking and pointing out @decsny. I looked at existing bindings to not re-invent the wheel. This example also lacks the justification: https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/display/ilitek%2Cili9xxx-common.yaml#L23

I really struggle to find a good justification as a description for this. It does not correspond to default values after reset, nor is it a manufacturer recommendation.

The default makes sense for when not interested in the rotation property, and it helps to avoid breaking existing sdd16xx usages.

Can you help find a suitable description? Or do I need to remove the default if it is not possible to give a good justification?

You can say rotation is off by default, for properties like this where one of the values corresponds to some kind of "none" meaning. The binding you linked is wrong for not clarifying this. Since you found it, please add a commit to fix it

Thanks for the excellent suggestion. I've updated this and added a commit for ili9xxx.

jfischer-no · 2024-05-28T18:02:02Z

drivers: display: ssd16xx: fix coding style

Reformat the drivers source code using clang-format tool.

This is not required and not acceptable, coding style is perfectly fine.

decsny · 2024-05-28T18:08:37Z

re-opening after inappropriate closure

faxe1008

The changes in the LVGL gluecode look ok but they are independent of the changes done in this pull request. They are simply there to make LVGL of the hardware rotation. (Note that this still does not allow for runtime rotation since the capabilities struct would need to be refetched upon rotation change).

In general it would be preferable to have pretty much atomic pull requests that only touch as much code as they need to. This involves not doing reformatting commits. Can you maybe adjust this PR to only add the rotation without the reformatting and open another PR for the LVGL code? The simpler you keep the pull requests the easier it is to get them reviewed and merged succesfully :^)

I chose this approach instead of software rotation because I need it for an ultra low power project. Rotation in steps of 0, 90, 180, 270 degrees is done by changing the data entry modes. This commit removes the orientation-flipped property as it is redundant. I made the assumption that the current driver implementation is right about controller X/Y versus display width/height. The display controller X parameter matches with the display height (dts) and display controller Y parameter matches with the display width (dts). It looks like display manufacturers choose to have it like this because a wider screen probably makes more sense. Tested on the reel_board with a 250x122 display (ssd1673 controller) and a custom PCB with a 200x200 display (ssd1681 controller). Also tested all orientations with various width/height dts overrides. Signed-off-by: Sven Depoorter <[email protected]>

The driver writes the scan entry mode to the display controller on each set_window call. Multiple calls to set_window can be needed to update a single frame buffer in the controller. There's a performance improvement by only setting the scan mode when the orientation is changed (set_orientation, see previous commit) and when the profile has changed. For the latter the scan_mode is lost on the controller due to a software reset. Signed-off-by: Sven Depoorter <[email protected]>

To align with udpated ssd16xx bindings. Signed-off-by: Sven Depoorter <[email protected]>

Snevzor · 2024-05-29T14:39:37Z

@jfischer-no and @faxe1008, the formatting commit has been dropped. Your input is welcome at #73465

@faxe1008 I've moved the LVGL stuff to: #73474

decsny

approving DT bindings

stale

carlescufi · 2024-05-30T09:13:26Z

@brgl @faxe1008 @fabiobaltieri @jfischer-no can we get a review on the code? Thanks!

danieldegrasse · 2024-05-30T16:06:09Z

drivers/display/ssd16xx.c

+		x_start = x / SSD16XX_PIXELS_PER_BYTE;
+		x_end = (x + desc->width - 1) / SSD16XX_PIXELS_PER_BYTE;
+		y_start = y;
+		y_end = (y + desc->height - 1);


Do we still need to write the scan_mode here, since we do it within the set_orientation function? IE will the scan mode setting persist between display frames, or does it need to be set every time?

Thanks for reviewing @danieldegrasse!

In the second commit I removed writing the scan_mode on each set_window call because I didn't find any indication in the datasheet that this should not persist between resets. So I tested it with the two mentioned display controllers and it worked so that seems to prove that the scan_mode indeed persists between display frames.
Because there is a software reset in the set_profile function I also added writing the scan_mode over there.

I do wonder why it was added to the set_window function in the first place instead of in the controller_init so I completely understand your doubts.
If anyone with a ssd16xx laying around could double check this, that would be great.
@andysan, I see you have made some amazing improvements to this driver in the past, would you mind checking this as well?

carlescufi · 2024-05-31T10:16:02Z

@danieldegrasse please take a look

danieldegrasse · 2024-05-31T17:46:19Z

@Snevzor when I run the display sample with this PR, I see logs like the following if I attempt to set the orientation to 90 degree rotation:

[00:00:00.382,385] <inf> sample: Display sample for ssd16xxfb@0
[00:00:00.938,629] <err> ssd16xx: Buffer out of bounds (width)
--- 84 messages dropped ---
[00:00:00.938,629] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,659] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,690] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,690] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,720] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,720] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,751] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,781] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,781] <err> ssd16xx: Buffer out of bounds (width)
[00:00:00.938,812] <err> ssd16xx: Buffer out of bounds (width

Given that the display sample works with rotation on other displays (such as ili9xxx based ones) I think we should target enabling this sample here as well, to make sure the API usage between each display is consistent when rotation is applied.

I chose this approach instead of software rotation because I need it for an ultra low power project.
Rotation in steps of 0, 90, 180, 270 degrees is done by changing
the data entry modes.

Does this imply that the display sample would need to change to accommodate the rotation setting?

Snevzor · 2024-06-01T11:15:40Z

@danieldegrasse thanks again for your review, testing, and patience. This is much appreciated.

You are responding to the parts I found most challenging, which is good! The reason these are challenging is that there seems to be a larger issue existing already.

Display drivers with rotation support (there aren't that many) that switch x/y resolution parameters in xxx_get_capabilities:

Display drivers with rotation support that don't switch x/y resolution parameters:

hx8394
nt35510
otm8009a
ssd16xx? ;)

There is also another open PR for adding rotation to st7789v (#72107). I've expressed my confusion about switching the parameters in this comment: #72107 (comment)
I concluded that LVGL takes into account display orientation (but needs an update in the glue code: #73474 / #73556) and that the driver should not switch x/y parameters to work well with LVGL.

I cannot test, but I would think that hw rotation with gc9x01x and ili9xxx together with LVGL will break when the LVGL glue code is updated.
I kindly invite @faxe1008 into this discussion because he is doing amazing work on this, and he seems exceptionally suited to identify the main thread and make the right decision to achieve uniform drivers.

I'm a little confused about "the display sample". Which one exactly?

LVGL sample: I have no issues with this driver and LVGL when either modules: lvgl: take into account initial display orientation #73474 / module: LVGL: Improve support for hardware display rotation #73556 is applied (as mentioned in the PR description). modules: lvgl: take into account initial display orientation #73474 is the bare minimum to have correct orientation from initialization (DTS). module: LVGL: Improve support for hardware display rotation #73556 adds some additional glue code for having display rotation at run time, which currently does not work for all display drivers because display capabilities can change when using HW rotation.
Do you still see those errors when applying the LVGL patch?
Have you tested it with a DTS overlay, or are you trying changing the rotation at run time?
To answer your question "Does this imply that the display sample would need to change to accommodate the rotation setting?": the LVGL sample does not need any change to accommodate for rotation at initialization. This is taken care of by the display driver and LVGL.
CFB sample: Looking into the various samples, it seems I've neglected the CFB sample and haven't tested that one with this display driver. My apologies! I've run that sample now, and it indeed fails for other orientations (dts) than 0. CFB just doesn't support rotating at all. I would think this would fail for all the drivers here.

P.S. Our time zone difference makes for a very slow conversation. It must be especially hard for you because of the multiple context switches. Also, I'm leaving on holiday from 4 to 18 June, so this might get stuck then. But I'd rather have this PR stay open until every issue around display rotation is resolved than having it merged ASAP. It's for a hobby project, and I can work with my own patches until this gets resolved.

danieldegrasse · 2024-06-01T17:50:04Z

There is also another open PR for adding rotation to st7789v (#72107). I've expressed my confusion about switching the parameters in this comment: #72107 (comment)
I concluded that LVGL takes into account display orientation (but needs an update in the glue code: #73474 / #73556) and that the driver should not switch x/y parameters to work well with LVGL.

Makes sense. I think we need to determine what makes the most sense here from the API perspective. Currently we have sort of left it up to the implementers which method a driver should use when performing rotation.

Personally, I'm partial to expecting the application to swap the X/Y coordinates, since this is what LVGL does already. @faxe1008 what are your thoughts here?

I'm a little confused about "the display sample". Which one exactly?

My apologies, I'm referring to samples/drivers/display. This is kind of a sample I treat as a "ground truth" for the display API- IE if colors in that sample are swapped in some way, the display driver is incorrect. I think once we settle on a system for display rotation, we should update the display sample to correctly account for rotation like this.

Snevzor · 2024-06-01T22:41:59Z

My apologies, I'm referring to samples/drivers/display. This is kind of a sample I treat as a "ground truth" for the display API- IE if colors in that sample are swapped in some way, the display driver is incorrect. I think once we settle on a system for display rotation, we should update the display sample to correctly account for rotation like this.

Wow, I wasn't even aware of that sample :p

It makes sense that if this is to be used as ground truth for the API that rotation support should be fully added. Preferably it should act as LVGL does because I assume LVGL is the most used display subsystem.

faxe1008 · 2024-06-02T19:10:55Z

Personally, I'm partial to expecting the application to swap the X/Y coordinates, since this is what LVGL does already. @faxe1008 what are your thoughts here?

I agree with you 👍 . No matter which method is preferred (swapping resolutions vs. not) you always need the triplet of x_resolution, y_resolution and orientation to determine where the rendering origin (in GUI space) is located. Only from my perspective the approach that orientation does not change the other parameters makes more sense imo.

The orientation of the display should serve as a guideline to any renderer how the content will be presented to the user. The rendering framework/method needs to accommodate for it (bounds checking, buffer management etc.).

Also, what I am thinking about is if maybe having set_orientation work in the native_sim (so SDL) would be a neat thing to have since not everybody has every display and this would be a way of making the expected orientation behavior easily demonstrable. What do you think?

Btw: @Snevzor thanks for splitting the PR up and bringing attention to this, code change wise this PR looks good imo. But since I do not own a display for that compatible to check and test the behavior I am hesitant on +1 this one :^)

danieldegrasse · 2024-06-03T15:14:19Z

Also, what I am thinking about is if maybe having set_orientation work in the native_sim (so SDL) would be a neat thing to have since not everybody has every display and this would be a way of making the expected orientation behavior easily demonstrable. What do you think?

I agree that implementing this API in the SDL driver is the way to go- that can be used as a "ground truth" for how the driver should behave.

My concern is that we have some technical debt here already- since we have display drivers behaving both ways in tree, there are likely downstream users relying on one behavior versus the other. As far as this PR, I think it is ok to merge as is (since the issue here is broader than this specific driver), but I think after 3.7 we should do the following:

standardize how we manage display orientation in Zephyr
update existing drivers to use this method
communicate this change to downstream users via the release notes and migration guide

I suggest making these changes after 3.7 because the time window until feature freeze is already rather tight- if you feel it would be better to try and standardize this before 3.7, I am open to trying but might need help converting/testing all display drivers in time. What are your thoughts here?

Snevzor · 2024-06-04T21:52:29Z

@danieldegrasse can you give a +1 on this one or do we need another reviewer?

danieldegrasse · 2024-06-04T21:58:22Z

@danieldegrasse can you give a +1 on this one or do we need another reviewer?

I'm good with the changes given the nebulous state of how display_set_orientation is supposed to work- we will need to standardize this after 3.7 though

github-actions · 2024-06-04T23:13:57Z

Hi @Snevzor!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

faxe1008 · 2024-06-06T18:28:52Z

I suggest making these changes after 3.7 because the time window until feature freeze is already rather tight- if you feel it would be better to try and standardize this before 3.7, I am open to trying but might need help converting/testing all display drivers in time. What are your thoughts here?

Yeah I think this is a post 3.7 change, the time window is too tight imo.
@Snevzor congrats to your first contribution :^)

zephyrbot added area: Display area: LVGL Light and Versatile Graphics Library Support area: Devicetree Binding PR modifies or adds a Device Tree binding labels May 27, 2024

zephyrbot requested review from brgl, danieldegrasse, decsny, faxe1008, galak, jfischer-no and pdgendt May 27, 2024 15:02

zephyrbot assigned galak May 27, 2024

Snevzor mentioned this pull request May 28, 2024

Better sync between LVGL and Zephyr for runtime hw rotation lvgl/lvgl#6294

Closed

decsny requested changes May 28, 2024

View reviewed changes

jfischer-no closed this May 28, 2024

decsny reopened this May 28, 2024

faxe1008 previously requested changes May 29, 2024

View reviewed changes

Snevzor mentioned this pull request May 29, 2024

doc: contribute guidelines: Explain when not to use clang-format #73465

Open

Snevzor added 3 commits May 29, 2024 15:52

drivers: display: ili9xxx: add justification for default rotation

93b5d12

To align with udpated ssd16xx bindings. Signed-off-by: Sven Depoorter <[email protected]>

Snevzor force-pushed the display_ssd16xx_set_orientation branch from ce38351 to 93b5d12 Compare May 29, 2024 14:23

Snevzor requested review from decsny and faxe1008 May 29, 2024 14:42

decsny approved these changes May 29, 2024

View reviewed changes

carlescufi unassigned galak May 30, 2024

Snevzor mentioned this pull request May 30, 2024

modules: lvgl: take into account initial display orientation #73474

Closed

danieldegrasse reviewed May 30, 2024

View reviewed changes

danieldegrasse self-assigned this May 30, 2024

Snevzor requested a review from danieldegrasse May 30, 2024 20:24

Snevzor mentioned this pull request Jun 1, 2024

drivers: display: st7789v: implement display_set_orientation API #72107

Closed

faxe1008 mentioned this pull request Jun 2, 2024

module: LVGL: Improve support for hardware display rotation #73556

Closed

danieldegrasse approved these changes Jun 4, 2024

View reviewed changes

nashif merged commit cabeaf1 into zephyrproject-rtos:main Jun 4, 2024

keith-zephyr mentioned this pull request Jun 5, 2024

doc: project roles: reviewer and assignee clarification #73691

Merged

danieldegrasse mentioned this pull request Dec 5, 2024

Standardize how display_set_orientation functions #82582

Open

drivers: display: ssd16xx: implement display_set_orientation #73360

drivers: display: ssd16xx: implement display_set_orientation #73360

Uh oh!

Conversation

Snevzor commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 27, 2024

Uh oh!

decsny May 28, 2024

Choose a reason for hiding this comment

Uh oh!

Snevzor May 28, 2024

Choose a reason for hiding this comment

Uh oh!

decsny May 28, 2024

Choose a reason for hiding this comment

Uh oh!

Snevzor May 29, 2024

Choose a reason for hiding this comment

Uh oh!

jfischer-no commented May 28, 2024

Uh oh!

decsny commented May 28, 2024

Uh oh!

faxe1008 left a comment

Choose a reason for hiding this comment

Uh oh!

Snevzor commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decsny left a comment

Choose a reason for hiding this comment

Uh oh!

carlescufi commented May 30, 2024

Uh oh!

danieldegrasse May 30, 2024

Choose a reason for hiding this comment

Uh oh!

Snevzor May 30, 2024

Choose a reason for hiding this comment

Uh oh!

carlescufi commented May 31, 2024

Uh oh!

danieldegrasse commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Snevzor commented Jun 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danieldegrasse commented Jun 1, 2024

Uh oh!

Snevzor commented Jun 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faxe1008 commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danieldegrasse commented Jun 3, 2024

Uh oh!

Snevzor commented Jun 4, 2024

Uh oh!

danieldegrasse commented Jun 4, 2024

Uh oh!

github-actions bot commented Jun 4, 2024

Uh oh!

faxe1008 commented Jun 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Snevzor commented May 27, 2024 •

edited

Loading

Snevzor commented May 29, 2024 •

edited

Loading

danieldegrasse commented May 31, 2024 •

edited

Loading

Snevzor commented Jun 1, 2024 •

edited

Loading

Snevzor commented Jun 1, 2024 •

edited

Loading

faxe1008 commented Jun 2, 2024 •

edited

Loading